Redesigned the framework to make it less vulnerable to native exceptions. #955
Conversation
|
can we please split formatting PR and real value PR? Mixing those two makes reviewing harder. Plus, why exactly do we do formatting as a hotfix into release branch? |
|
Basically, everything that @ilammy said here |
There was a problem hiding this comment.
This is a great PR, thank you @radetsky for implementing all the changes.
I have couple of questions / concerns.
-
Error handling on iOS side. I can be wrong, but from my perspective, we should check "if error != nil" as well.
-
Breaking changes. As I understood, we switched from the "exception" based flow to the "error" based flow. I'd consider this a breaking change that affects 0.14.X users.
Keeping in mind your desire to improve SecureMessage interface (remove private/public key where they are not needed), what do you think about making this PR a part of 0.15.0 instead?
...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java
Show resolved
Hide resolved
...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java
Outdated
Show resolved
Hide resolved
...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java
Show resolved
Hide resolved
| NSError *error; | ||
|
|
||
| cell = [self newSealMode:symmetricKey error:&error]; | ||
| if (cell == nil) { |
There was a problem hiding this comment.
shouldn't we check for error here as well? is it possible to have error and non-nil cell?
There was a problem hiding this comment.
#951 (comment)
We already discussed it with @ilammy
He asked me to make it right.
There was a problem hiding this comment.
@ilammy asked you to check that value is not nil, not to stop checking if error is present.
I suggest you check both, that "if value == nil OR error != nil".
My suggestions are based on a certain paranoia level, the "Of course, it is never null in the current version of the code" argument, and Cocoa Error handling guides:
if (!theData && theError)
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ErrorHandlingCocoa/CreateCustomizeNSError/CreateCustomizeNSError.html#//apple_ref/doc/uid/TP40001806-CH204-BAJIIGCC
if (xmlDoc == nil && err)
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ErrorHandlingCocoa/RecoverFromErrors/RecoverFromErrors.html#//apple_ref/doc/uid/TP40001806-CH206-BCIDEGGF
Please pay attention that Cocoa guides use "AND" while I suggesting using "OR": the object was not created OR the error occurred.
There was a problem hiding this comment.
@radetsky didn't get any comments from you here.
|
We do not do breaking changes. iOS principal error handling was reworked because of suggestions from @ilammy and Apple Developer documentation. But the interface of the library was not changed. I spent a lot of time writing an application with tests to check and prove it. |
Does the previous published Themis RN wrapper version uses the exception flow? |
|
@radetsky does the previous published Themis RN wrapper version uses the exception flow? Because now I see that we switched to the error flow. |
|
okay, @radetsky says that switching between exception flow and error flow affects only internal logic, the interface for the user is the same. let's merge |
…ons. (#955) * redesigned the framework to make it less vulnerable to native exceptions. Second try. * restore previous format * no more short names
…ons. (#955) * redesigned the framework to make it less vulnerable to native exceptions. Second try. * restore previous format * no more short names
Second attempt. Excuse me for the formatting issue, but my eyes already can not work with 2 spaces.